Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move optdata and version file generation up to MSBuild from build-runtime #58674

Merged
merged 47 commits into from
Oct 27, 2021
Merged

Move optdata and version file generation up to MSBuild from build-runtime #58674

merged 47 commits into from
Oct 27, 2021

Conversation

jkoritzinsky
Copy link
Member

Move optdata and version file generation from the native build scripts up into the managed scripts. The native build scripts will now by default copy a fallback version file in place if the version files do not exist and will disable PGO if no pgo file path is passed in to the build-runtime scripts.

This removes all cases of build-runtime calling into MSBuild, which removes the last MSBuild-in-Batch/Bash-in-MSBuild process nesting we have in the product build (there's still some in the test build).

This PR also renames the clr.dactools subset to clr.nativeprereqs to reflect its expanded role.

@jkoritzinsky
Move optdata and version file generation from the native build script…
bff7536 
…s up into the managed scripts. The native build scripts will now by default copy a fallback version file in place if the version files do not exist and will disable PGO if no pgo file path is passed in to the build-runtime scripts.

This removes all cases of build-runtime calling into MSBuild.
@jkoritzinsky
Update CI to use the new prereqs model.
a8b692c
@jkoritzinsky
Remove some old MSBuild arg pass-through that's no longer needed as b…
1813a73 
…uild-runtime no longer calls into any MSBuild processes.
@ghost
Copy link

ghost commented Sep 4, 2021

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Move optdata and version file generation from the native build scripts up into the managed scripts. The native build scripts will now by default copy a fallback version file in place if the version files do not exist and will disable PGO if no pgo file path is passed in to the build-runtime scripts.

This removes all cases of build-runtime calling into MSBuild, which removes the last MSBuild-in-Batch/Bash-in-MSBuild process nesting we have in the product build (there's still some in the test build).

This PR also renames the clr.dactools subset to clr.nativeprereqs to reflect its expanded role.

Author: jkoritzinsky
Assignees: -
Labels:

area-Infrastructure-coreclr
Milestone: -

@jkoritzinsky
Copy link
Member Author

cc: @jkotas this is the build change we talked about earlier today.

@agocke This PR should have an impact on no-op incremental build times. Once I get it green and have CI working correctly, I'll get some numbers for you.

ViktorHofer
ViktorHofer reviewed Sep 7, 2021
View reviewed changes
eng/pipelines/coreclr/templates/build-job.yml
Comment on lines +184 to +186
# Build/Generate native prerequisites
- script: $(Build.SourcesDirectory)$(dir)build$(scriptExt) -subset clr.nativeprereqs $(crossArg) -arch $(archType) $(osArg) -c $(buildConfig) $(officialBuildIdArg) -ci /bl:$(Build.SourcesDirectory)artifacts/log/$(buildConfig)/CoreCLRNativePrereqs.binlog
displayName: Build and generate native prerequisites
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the common msbuild entry point to build the CoreCLR runtime and by that avoid the msbuild invocation here and the extra AzDO specific OutputPgoPathForCI target?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke with @jkotas before doing this work and he said that he still doesn't want us to call into CMake/Ninja from MSBuild if we can avoid it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sharing my thoughts from my conversation with Jeremy:

  • Designs where one build driver (msbuild) is wrapping another build driver (ninja) are very hard to debug.
  • I think it is important to have a reasonable way to build the unmanaged runtime without a working managed runtime for bootstrapping on new platforms.
  • Things like version files are non-essential and it is easy to replace them with placeholder files. I do not see a problem with using C# to generate them. I would actually prefer to use C# to generate them. It would be nice to get rid of the python dependency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

* It would be nice to get rid of the python dependency.

For eventpipe generator as well (can be replaced with C++ shim/macros as done for numa and openssl shims). We could also try to get rid of perl dependency, we have some headers generators still written in perl (which not many folks use in this day and age).. 🙂

@jkoritzinsky jkoritzinsky force-pushed the no-msbuild-in-build-runtime branch from c82f25d to 790204b Compare September 8, 2021 18:47
hoyosjs
hoyosjs reviewed Sep 8, 2021
View reviewed changes
@jkoritzinsky
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@runfoapp runfoapp bot mentioned this pull request Oct 12, 2021
Closed
@runfoapp runfoapp bot mentioned this pull request Oct 18, 2021
Closed
hoyosjs
hoyosjs approved these changes Oct 19, 2021
View reviewed changes
@runfoapp runfoapp bot mentioned this pull request Oct 26, 2021
Closed
@hoyosjs hoyosjs merged commit 4682098 into dotnet:main Oct 27, 2021
@hoyosjs
Copy link
Member

hoyosjs commented Oct 27, 2021

Thanks for this @jkoritzinsky

@jkoritzinsky jkoritzinsky deleted the no-msbuild-in-build-runtime branch October 27, 2021 17:05
@BruceForstall
Copy link
Member

@jkoritzinsky It looks like this breaks the JIT rolling build, e.g.:

C:\gh\runtime4\src\coreclr\utilcode\utilmessagebox.cpp(205): error C2220: the following warning is treated as an error
C:\gh\runtime4\src\coreclr\utilcode\utilmessagebox.cpp(205): warning C4003: not enough arguments for function-like macro invocation 'QUOTE_MACRO_L_HELPER'
C:\gh\runtime4\src\coreclr\utilcode\utilmessagebox.cpp(205): error C2065: 'L': undeclared identifier

https://dev.azure.com/dnceng/internal/_build?definitionId=902

which is built in a clean enlistment with:

src\coreclr\build-runtime.cmd x64 checked -component alljits

cc @dotnet/jit-contrib

@jkoritzinsky
Copy link
Member Author

@BruceForstall I'll put out a PR to generate the version files for that pipeline to unblock it. Once that's out, I'll investigate what part of the version fallback doesn't work.

@BruceForstall
Copy link
Member

Thanks. btw, it's trivial to repro this on a clean enlistment (no artifacts folder) by running that build command.

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Oct 28, 2021 •
edited
Loading

#60981 should have a fix. Is there a public pipeline I can run to validate the fix or are all of the affected pipelines internal-only?

@BruceForstall
Copy link
Member

You can trigger the "runtime-coreclr superpmi-replay" pipeline, which also uses build-jit-job.yml

@BruceForstall
Copy link
Member

That's a 2+ hour pipeline, but the build is a pre-req that happens quickly, and first.

@BruceForstall
Copy link
Member

So is the model that you can't call build-runtime.cmd on a "raw" enlistment anymore?

Should the pipeline invoke the root build.cmd, e.g.,

build.cmd -c Checked -arch x64 -subset clr.alljits

instead?

@BruceForstall
Copy link
Member

Doing that does a few things (dotnet CLI install, wasm restores?) before invoking "C:\gh\runtime4\src\coreclr\build-runtime.cmd" -x64 -checked -component alljits. It notably doesn't output anything about building version files (and I don't think we generate any log files anymore, so you can't actually see individual steps that are performed), but it works.

@hoyosjs
Copy link
Member

hoyosjs commented Oct 28, 2021

build-runtime.cmd won't work on a raw enlistment if you're trying to do a full native build. It should work for the jits though. @jkoritzinsky shouldn't this pass in that case? Just copy the phony files and skip PGO?

@jkoritzinsky
Copy link
Member Author

Yes, but it looks like there's a bit with the fallback files that doesn't work quite right that I missed in my testing.

@hoyosjs
Copy link
Member

hoyosjs commented Oct 28, 2021

I see https://github.com/dotnet/runtime/blob/main/eng/native/version/runtime_version.h

RuntimeProductVersion is blank @jkoritzinsky was this intentional? It's supposed to be version. We could just have it be the 42.42.42... thing that arcade uses

@ghost ghost locked as resolved and limited conversation to collaborators Nov 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@am11 am11 am11 approved these changes

@jkotas jkotas jkotas left review comments

@hoyosjs hoyosjs hoyosjs approved these changes

@imhameed imhameed Awaiting requested review from imhameed

@lambdageek lambdageek Awaiting requested review from lambdageek

@marek-safar marek-safar Awaiting requested review from marek-safar

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

@vargaz vargaz Awaiting requested review from vargaz

@ViktorHofer ViktorHofer Awaiting requested review from ViktorHofer

Assignees
No one assigned
Labels
area-Infrastructure-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jkoritzinsky @hoyosjs @BruceForstall @am11 @jkotas @ViktorHofer